Skip to content

Conversation

@yomybaby
Copy link
Member

@yomybaby yomybaby commented Sep 1, 2025

Fix Query Parameter State Management and Persistence

Resolves #4251 (FR-1446)

Summary

This PR fixes the state persistence issue in the query parameters hook where pagination and filter values were not persisting correctly after page refresh, and addresses cross-page state pollution issues.

Problem

Users experienced issues where:

  • Pagination and filter values would revert to initial state after page refresh
  • Global state was shared between different pages causing state pollution
  • Memory leak concerns with atom lifecycle management

Solution

  1. Refactored State Management: Changed from global atom to page-specific state management
  2. Fixed State Persistence: URL query params now serve as initial values, with internal state maintaining values during navigation
  3. Resolved Cross-Page Issues: Each page now has independent query parameter state that resets when navigating to a different page
  4. Improved Naming: Renamed useDeferredQueryParams to useTransitionSafeQueryParams to better reflect its purpose of handling React transitions safely
  5. Memory Management: Proper atom lifecycle management prevents memory leaks

Technical Implementation

  • Uses page-scoped atoms that automatically reset when pathname changes
  • URL query parameters serve as initial values only
  • Internal state manages subsequent updates and synchronizes with URL
  • Compatible with React transitions and concurrent rendering
  • Same page components can share state, but different pages are isolated

Files Changed

  • react/src/hooks/useDeferredQueryParams.tsx (deleted)
  • react/src/hooks/useTransitionSafeQueryParams.tsx (new implementation)
  • Updated imports in 5 component files

Testing

  • Lint checks pass
  • Pagination values persist after page refresh
  • Filter values persist after page refresh
  • No cross-page state pollution
  • Memory usage remains stable during navigation

@github-actions github-actions bot added the size:L 100~500 LoC label Sep 1, 2025
Copy link
Member Author

yomybaby commented Sep 1, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • flow:merge-queue - adds this PR to the back of the merge queue
  • flow:hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has required the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@yomybaby yomybaby changed the title Revert "fix(FR-1401): fix state persistence issue in useDeferredQueryParams hook (#4179)" fix(FR-1431): fix state persistence issue in query params hook Sep 1, 2025
@yomybaby yomybaby changed the title fix(FR-1431): fix state persistence issue in query params hook fix(FR-1431): fix state persistence issue in query params hook Sep 1, 2025
@yomybaby yomybaby changed the title fix(FR-1431): fix state persistence issue in query params hook fix(FR-1431): fix state persistence issue in query params hook Sep 1, 2025
@yomybaby yomybaby force-pushed the fix/use-deferred-query branch 3 times, most recently from 7b66114 to cca9416 Compare September 2, 2025 07:55
@github-actions
Copy link

github-actions bot commented Sep 2, 2025

Coverage report for ./react

St.
Category Percentage Covered / Total
🔴 Statements
4.59% (-0% 🔻)
501/10916
🔴 Branches
3.7% (+0% 🔼)
285/7697
🔴 Functions
2.62% (+0% 🔼)
90/3434
🔴 Lines
4.56% (-0% 🔻)
487/10688
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🔴
... / useTransitionSafeQueryParams.tsx
3.03% 0% 0% 3.13%

Test suite run success

114 tests passing in 13 suites.

Report generated by 🧪jest coverage report action from 2b72266

@yomybaby yomybaby requested a review from nowgnuesLee September 4, 2025 05:59
@yomybaby yomybaby force-pushed the fix/use-deferred-query branch from cca9416 to 99d7fac Compare September 5, 2025 02:31
@yomybaby yomybaby marked this pull request as ready for review September 5, 2025 03:18
@Copilot Copilot AI review requested due to automatic review settings September 5, 2025 03:18
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes state persistence issues in query parameters by replacing the global useDeferredQueryParams hook with a new page-scoped useTransitionSafeQueryParams hook. The changes prevent cross-page state pollution and ensure pagination/filter values persist correctly after page refresh.

Key changes:

  • Replaced global state management with page-specific state isolation
  • Renamed hook to better reflect its purpose of handling React transitions safely
  • Added proper atom lifecycle management to prevent memory leaks

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
react/src/hooks/useTransitionSafeQueryParams.tsx New implementation with page-scoped state management and context provider
react/src/hooks/useDeferredQueryParams.tsx Removed old global state implementation
react/src/App.tsx Added PageQueryParamAtomProvider wrapper for proper state isolation
react/src/pages/*.tsx Updated imports to use new hook name across 3 page components
react/src/components/AgentSummaryList.tsx Updated import to use new hook name
react/src/hooks/reactPaginationQueryOptions.tsx Updated import to use new hook name

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +55 to +66
// Create page-specific atoms that reset when pagePath changes
const localQueryParamsAtom = useMemo(() => {
// Internal state atom for this specific page
let hasLoadedFromURL = false;

// Derived atom that manages the query parameters
const queryParamsAtom = atomWithDefault((get) => {
const pageQueryParamDelta = get(pageQueryParamDeltaAtom);

// Use URL query params on first access, then use internal state
if (!hasLoadedFromURL) {
hasLoadedFromURL = true;
Copy link

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hasLoadedFromURL variable is declared inside the useMemo callback but shared across re-renders. This will cause the variable to reset on every render, making the URL loading logic unreliable. Move this variable outside the useMemo or use a ref to maintain its state across renders.

Suggested change
// Create page-specific atoms that reset when pagePath changes
const localQueryParamsAtom = useMemo(() => {
// Internal state atom for this specific page
let hasLoadedFromURL = false;
// Derived atom that manages the query parameters
const queryParamsAtom = atomWithDefault((get) => {
const pageQueryParamDelta = get(pageQueryParamDeltaAtom);
// Use URL query params on first access, then use internal state
if (!hasLoadedFromURL) {
hasLoadedFromURL = true;
// Persist flag across renders and atom invocations
const hasLoadedFromURLRef = useRef(false);
// Create page-specific atoms that reset when pagePath changes
const localQueryParamsAtom = useMemo(() => {
// Derived atom that manages the query parameters
const queryParamsAtom = atomWithDefault((get) => {
const pageQueryParamDelta = get(pageQueryParamDeltaAtom);
// Use URL query params on first access, then use internal state
if (!hasLoadedFromURLRef.current) {
hasLoadedFromURLRef.current = true;

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I share the same curiosity as this review.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review is incorrect. The hasLoadedFromURL variable is intentionally designed to reset when useMemo creates a new atom instance. This ensures each atom gets proper initial URL loading, then switches to using pageQueryParamDelta. Moving it outside would break this intended behavior - it's a feature, not a bug.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplified the implementation by removing the hasLoadedFromURL logic entirely. The hook now directly merges query with pageQueryParamDelta, making the original review concern obsolete.

Comment on lines +130 to +133
const PageQueryParamDeltaAtomContext = createContext<
PrimitiveAtom<Record<string, any>> | undefined
>(undefined);
Copy link

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The context default value should not be undefined as it creates potential runtime errors. Consider providing a proper default atom or throwing an error in the context creation to make the contract clearer.

Suggested change
const PageQueryParamDeltaAtomContext = createContext<
PrimitiveAtom<Record<string, any>> | undefined
>(undefined);
const PageQueryParamDeltaAtomContext = createContext<PrimitiveAtom<Record<string, any>>>(
atom(() => {
throw new Error(
'PageQueryParamDeltaAtomContext must be used within <PageQueryParamAtomProvider>'
);
})
);

Copilot uses AI. Check for mistakes.
Comment on lines +55 to +66
// Create page-specific atoms that reset when pagePath changes
const localQueryParamsAtom = useMemo(() => {
// Internal state atom for this specific page
let hasLoadedFromURL = false;

// Derived atom that manages the query parameters
const queryParamsAtom = atomWithDefault((get) => {
const pageQueryParamDelta = get(pageQueryParamDeltaAtom);

// Use URL query params on first access, then use internal state
if (!hasLoadedFromURL) {
hasLoadedFromURL = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I share the same curiosity as this review.

@yomybaby yomybaby requested a review from nowgnuesLee September 8, 2025 12:40
@yomybaby yomybaby marked this pull request as draft September 8, 2025 12:46
@yomybaby yomybaby force-pushed the fix/use-deferred-query branch from 99d7fac to 2f52a79 Compare September 8, 2025 12:56
@yomybaby yomybaby marked this pull request as ready for review September 8, 2025 13:00
Comment on lines 127 to 128
throw new Error(
'usePageQueryParamDeltaAtom must be used within <PageQueryParamAtomP44rovider>',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new Error(
'usePageQueryParamDeltaAtom must be used within <PageQueryParamAtomP44rovider>',
throw new Error(
'usePageQueryParamDeltaAtom must be used within <PageQueryParamAtomProvider>',

@yomybaby yomybaby force-pushed the fix/use-deferred-query branch from 2f52a79 to 2b72266 Compare September 9, 2025 02:19
@yomybaby yomybaby changed the title fix(FR-1431): fix state persistence issue in query params hook fix(FR-1446): refactor Sep 9, 2025
@yomybaby yomybaby changed the title fix(FR-1446): refactor fix(FR-1446): refactor useDeferredQueryParams Sep 9, 2025
@yomybaby yomybaby marked this pull request as draft September 9, 2025 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100~500 LoC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Re-implement a hook for managing query parameter state that supports React transitions.

2 participants